HDDS-6184: EC: put command should create EC key if bucket is EC #2990
Conversation
umamaheswararao
force-pushed
the
HDDS-6184
branch
from
3bfde0e to
d709e7f
Compare
There was a problem hiding this comment.
Aren't this two condition remained here accidentally? Maybe I miss something, but if I am correct, we do the same in this two ifs as inside the method OzoneClientUtils#validateAndGetClientReplicationConfig(), which we call from here.
There was a problem hiding this comment.
Good catch. I already moved these checks inside OzoneClientUtils.validateAndGetClientReplicationConfig.
There are just redundant here and removed them now.
There was a problem hiding this comment.
Shouldn't we open the key we have created by executing the shell command to run putkey earlier instead of creating it? (Also that would mean we get an OzoneInputStream here and we can check its type.)
There was a problem hiding this comment.
You are right. I supposed to reopen key and check type. I did that correction now. Thanks
There was a problem hiding this comment.
nit: we can use asserEquals here on the class instances
There was a problem hiding this comment.
changed this assert to check types now on KeyDetails.
umamaheswararao
left a comment
umamaheswararao
left a comment
There was a problem hiding this comment.
Thanks @fapifta for the review. I have addressed you comments and pushed another commit.
There was a problem hiding this comment.
Good catch. I already moved these checks inside OzoneClientUtils.validateAndGetClientReplicationConfig.
There are just redundant here and removed them now.
There was a problem hiding this comment.
You are right. I supposed to reopen key and check type. I did that correction now. Thanks
There was a problem hiding this comment.
changed this assert to check types now on KeyDetails.
There was a problem hiding this comment.
Thank you @umamaheswararao for the adjustment. +1 after CI
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneClientUtils.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneConfigUtil.java
Outdated
Show resolved
Hide resolved
umamaheswararao
force-pushed
the
HDDS-6184
branch
from
f133072 to
40a956e
Compare
sodonnel
left a comment
sodonnel
left a comment
There was a problem hiding this comment.
LGTM - Thanks for adding the unit tests. Its just good to have them incase someone changes something by mistake in the future.
4 participants
What changes were proposed in this pull request?
Handled the client side configuration precedence and passing the null to server if no client configs available. Before this patch, client always passing values.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6184
How was this patch tested?
Added test to check the behavior.